-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backend fetch http #1706
Backend fetch http #1706
Conversation
What is the feature?Adding fetch http to the backend Does it have an issue?No. Looks like its a bullet point of #1691 What is it doing?ExplanationI would expect this to be pretty similar to the front end, in that we should So all of those weird header things that we were worrying about we will also Questions
AnswersWhat is the api for http requests?Under the hood we will be calling the management canister through the rust api. Basically http_request makes an http request. Pretty straight forward. Let's see I am also told to look at the interface spec for more information. We will take a look at that later. CanisterHttpRequestArgumentThis has
I am hoping that all of that will make more sense after reading the interface spec, so let's jump to that now. What can we learn from the Interface SpecThe canister should ait to issue idempotent requests. I think that's on our Ah, so the transform function is used to sanitize a response so that things Oh, and the canister will only have access to the request after it's sanitized, The spec owns up to only supported GET, HEAD, and POST methods. With the For security reasons only https is supported. The max size for a request or response is 2_000_000B. That makes sense, it's Cool, I got a lot more questions from that... Let's keep plugging along on those What does the fetch api look like for the http protocol?In a simple example it looks like this const response = await fetch(
"http://url.domain",
{
method: "POST",
headers: {
"My-Header-Name": "My-Header-Value"
},
body: [1, 2, 3, 4, 5, 6, 7]
}
); But I'm guessing I'm going to need more that just a little easy start up demo I recently learned that undici is the For right now I would expect us only to implement the most common API features. Even more specifically they have a section about Syntaxfetch(resource)
fetch(resource, options) resource is either a url (a string or anything with a stringifier (included URL) that will resolve to a url essentially) or a Request object. And then options is an object with a bunch of options. If I can just look through what the Request object is and briefly cover all of Request ObjectIt has 14 properties.
I think most of those are pretty straight forward (except the ones with ?)... I guess I would expect our code to be able to parse all of that and add it to Options
At this point I am feeling a little overwhelmed with each of these. So I am ExpectationsTests
CodeCode should
|
ResponsesI almost forgot to look at responses new Response()
new Response(body)
new Response(body, options) body is either null (by default) or one of
options are
In rust for http_request the response looks like Headers are going to be like this pub struct HttpHeader {
pub name: String,
pub value: String,
} I would expect the code to also properly covert from rust response to http response. I think I would expect it to always be a blob body, with a Headers object, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Results
Rubric
- Consistency (Same as the rest of the code and itself)
- There are a handful of new
any
types.
- There are a handful of new
- Readability (Easy to understand)
- return value for the http protocol is a little hard to read at first glance
- could be more declarative
- azleFetch is starting to get pretty long, could we break it up into smaller functions
- slitting along protocol lines seems like a good natural breaking point
- I like the updates to getBalance and getBlockByNumber in the ethereum json rpc example
- return value for the http protocol is a little hard to read at first glance
- Maintainability (modular, reusable, and simple)
- I like the reuse of azleFetch in the http protocol implementation
- Correctness (as bug free as reasonable)
- Discussed in detail below
- Code level implications (code coverage, error coverage, conditionals coverage, efficiency, etc)
- Discussed in detail below
- Quality of tests and documentation (thinking about how others will use it)
- Discussed in detail below
Code
Lets start with responses so we don't forget them again. Responses are not
quite what I was expecting, I have added a comment to that effect. Overall
I do like the http implementation, just using the icp protocol to call the
management canister as we had been doing before. I guess on that note we used
to have responses from azleFetch that were never like a Response object and it
seemed to work then ¯\_(ツ)_/¯
I like the api for adding a transform function. I'm just a little nervous about
how to best teach people what a transform method is, why they need it, and
making sure that they have added it as a query or update method.
There was a TODO to address unsupported methods. I think that should be pretty
easy to add and would be beneficial. Or at least as beneficial as the odds of
someone doing a method besides POST or GET.
It allows http calls. It shouldn't
Exporting transform functions
Knowing what I now do about transform functions the developer must provide one
if one is needed. That is not something we can do. Hopefully they don't need
one, but what happens if they do and they don't provide one, or they make one,
but they don't add it as a query method?
Implement a good selection the most common api features of node's global fetch
I wrote this one before diving into the mozilla developer docs. I'm not sure it
actually applies like I thought it would when I wrote it. because the protocol
is pretty straight forward. We would just need to make sure we cover all of the
options. Or that we can pass the buck to the management canister for not
supported all of the options.
What options do we support?
It looks like almost none of them. The common ones were
- body
- cache
- credentials
- headers
- integrity
- method
- mode
- redirect
- referrer
- referrerPolicy
- signal
We don't currently support URL or Request objects for the fetch input, but I
think those would be very easy to add support for, especially given that we just
ignore the options that aren't body, headers, and method.
Tests
- Do I want to see each option represented in at least one test?
- Right now options aren't supported so I won't expect these until we add
them
- Right now options aren't supported so I won't expect these until we add
- I might want to see the tests for each of ic.call and icp protocol and http
protocol- Actually I don't think we need this because http is calling the icp
protocol so leaving it as is should provide this coverage.
- Actually I don't think we need this because http is calling the icp
- I want to see a test with a url string URL object and a Request object.
- I think we should support this and add tests for it.
- I want to see a test with a transform function
- We already have that in both of the examples that this pr touches.
- I want to see a test with each of the three allowed methods
- We have GET and POST tests, but no HEAD tests.
- Representatives of the different header approaches
- We don't have any tests for headers and consequently we didn't catch
that they are being completely disregarded
- We don't have any tests for headers and consequently we didn't catch
- Maybe I want to see test that hit the url length max and header maxes
- We don't currently enforce those limits, if we don't then it's probably
not important, and would be on dfinity to test those, if we do then we
should test them.
- We don't currently enforce those limits, if we don't then it's probably
Misc Concerns
It seems like doing a canister side fetch call is going to very quickly start to
require some deep IC knowledge.
- Transform functions (which brings query/update functions (which brings candid)), cycle costs, replication and idempotent requests
34514e6
to
f814835
Compare
f814835
to
43ffb5c
Compare
…r fetch, clean up ethers fetch integration
… and response headers and status codes
The purpose of this PR is to provide general-purpose
http
capabilities fromfetch
. The developer should not need to understand that they are on ICP to do basicGET
andPOST
requests. These are currently the only types of requests that are supported. I'm not sure how important it is to provide nice errors if the user tries to use unsupported methods, headers, etc...they will still get an error by the system at least for the methods I would guess.They shouldn't have to do anything ICP-specific as I said, unfortunately there are some settings on http requests (the management canister is the underlying mechanism to actually perform these requests) that the developer must set. So we have come up with a global method on
ic
to set all of these settings. They can call this before making http requests to set those settings.If they don't call this, they will use default settings that could be very expensive and perhaps not calibrated correctly. I believe I have the default settings pretty good for most use cases, as in it should work and not throw errors.
Generally they should set the global settings. We would like to push DFINITY/ICP to remove the need to specify these extra settings.
I have some basic tests, but maybe now or maybe in another PR it would probably be good to add a more robust set of tests.
This is a basic implementation, I'm not sure we should go for the entire fetch API right now, but the happy path. For example on the frontend fetch we already decided not to implement accepting the Request object, we might want to do the same here.